Skip to content

Add support for containers of move only typed values to range. #3586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Dec 18, 2018
Merged

Add support for containers of move only typed values to range. #3586

merged 7 commits into from
Dec 18, 2018

Conversation

thomasspriggs
Copy link
Contributor

@thomasspriggs thomasspriggs commented Dec 17, 2018 •
edited
Loading

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

romainbrenguier
romainbrenguier approved these changes Dec 17, 2018
View reviewed changes
unit/util/range.cpp Outdated
REQUIRE(both_inputs[19].value == 10);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ missing new line

unit/util/range.cpp Outdated
input1.emplace_back(i);
std::vector<move_onlyt> input2;
for(int i = 1; i <= 10; ++i)
input2.emplace_back(i);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ input2 is not used in all the tests, consider moving it closer to the test actually using it (maybe in a second GIVEN)

thk123
thk123 reviewed Dec 17, 2018
View reviewed changes
Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the commit messages contain a little more info why these things are required.

tautschnig
tautschnig approved these changes Dec 17, 2018
View reviewed changes
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor bits left as noted by reviewers.

src/util/range.h
@@ -357,12 +357,11 @@ ranget<iteratort> make_range(iteratort begin, iteratort end)
return ranget<iteratort>(begin, end);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on the commit message: s/in oder/in order/

@thomasspriggs
Copy link
Contributor Author

I have addressed the review comments. I have also added better move support and corresponding unit tests for the map operation, as I realised I had previously missed this.

allredj
allredj reviewed Dec 17, 2018
View reviewed changes
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚫
This PR failed Diffblue compatibility checks (cbmc commit: cdcd544).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/95016060
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

  • the cbmc commit has disappeared in the mean time (e.g. in a force-push)
  • the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

@thomasspriggs
Allow make_range to construct a range of non-const values
f010ebf 
This is required in order to allow values in the constructed range to be
moved from.
@thomasspriggs
Add non-const overloads of the * and -> for filter_iteratort
181becf 
This adds support for moving from the values accessible through the
result of `ranget.filter`.
@thomasspriggs
Add non-const overloads of the * and -> for concat_iteratort
16bc92f 
This adds support for moving from the values accessible through the
result of `ranget.concat`.
@thomasspriggs
Update map_iteratort to use a shared_ptr for current
81e4a93 
This allows mapping into move only types. This was not previously
possible, because the copy constructor of `map_iteratort` copied the
value pointed to by its unique pointer in its `current` field. This can
also be considered to be more correct, because copying an iterator
should be expected to yield two copies, which point to the same value,
rather than two copies, which point to two separate values.
@thomasspriggs
Add non-const overloads of the * and -> for map_iteratort
a6832e3 
This adds support for moving from the values accessible through the
result of `ranget.map`.
@thomasspriggs
Add unit test on ranges of move only values
0b73746
@thomasspriggs
Copy link
Contributor Author

I have rebased in order to re-check test-gen compatibility.

thk123
thk123 reviewed Dec 18, 2018
View reviewed changes
Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New changes look good and previous comments addressed - if possible I'd like to see two more tests for map

unit/util/range.cpp
{
const std::vector<int> input{1, 2, 3, 4, 5};

THEN("The vector can be mapped into a range of move-only types")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works - you should add a test that maps from a move only type to a regular type, and one that moves the same type through the map

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test which "maps from a move only type to a regular type" already exists. As we discussed, you missed it when reviewing. The f passed into map can't move from its parameter, because the parameter has to be a const reference. I have expanded the doxygen to document the extent of the support for move-only types and its limitations.

allredj
allredj reviewed Dec 18, 2018
View reviewed changes
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️
Passed Diffblue compatibility checks (cbmc commit: 0b73746).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/95095292

@thk123
Copy link
Contributor

thk123 commented Dec 18, 2018

Also @romainbrenguier should definitely re-review the unique_ptr -> shared_ptr before merging.

@thomasspriggs
Expand doxygen on ranget.map
bbc2016 
The new doxygen explains the extent and limitations of `ranget.map`'s
support of move-only types.
allredj
allredj reviewed Dec 18, 2018
View reviewed changes
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️
Passed Diffblue compatibility checks (cbmc commit: bbc2016).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/95102012

romainbrenguier
romainbrenguier approved these changes Dec 18, 2018
View reviewed changes
Copy link
Contributor

@romainbrenguier romainbrenguier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from unique to shared pointer looks good

@thomasspriggs thomasspriggs merged commit b280689 into develop Dec 18, 2018
@thomasspriggs thomasspriggs deleted the tas/range_move branch December 18, 2018 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thk123 thk123 thk123 left review comments

@allredj allredj allredj left review comments

@tautschnig tautschnig tautschnig approved these changes

@romainbrenguier romainbrenguier romainbrenguier approved these changes

@kroening kroening Awaiting requested review from kroening kroening is a code owner

@peterschrammel peterschrammel Awaiting requested review from peterschrammel peterschrammel is a code owner

@pkesseli pkesseli Awaiting requested review from pkesseli

@smowton smowton Awaiting requested review from smowton

Assignees

@thomasspriggs thomasspriggs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thomasspriggs @thk123 @tautschnig @romainbrenguier @allredj